[Assets] Implement `pallet-assets-holder` #4530

pandres95 · 2024-05-21T05:43:03Z

…nce based on whether there's held assets or not

…th the tokens' _balance components_ model. On the most recent documentation about tokens (both _fungible_ and _fungibles_ (sets)), the model for calculating the different balance components is explained. The prior implementation of `pallet-assets` featured a weird definition of how to handle them that was not in line with the expected (see <https://paritytech.github.io/polkadot-sdk/master/frame_support/traits/tokens/fungible/index.html#holds-and-freezes>) definition of tokens. This commit changes this implementation for methods `reducible_balance` and `can_decrease` to introduce the calculation of `spendable` balance, and the consequences derived of decreasing a balance that may include both `frozen` and `held` balances.

… working together

bkchr · 2024-07-24T09:24:24Z

@joepetrowski @muharem can you please take a look at this and if we need this?

muharem

@pandres95 I do not really understand the plan. I see only first step in the issue. also are you sure we need this new trait? can we do the same with fungibles traits? I see that we have a similar FrozenBalance trait, but it probably was introduced before the fungibles was. can we bring the hold implementation with a single PR? it would make it more clear and we get it faster. this PR wont bring anything useful, only breaking changes.

pandres95 · 2024-07-25T11:39:47Z

Sure! Can complete the second step in the same PR.

The reason behind this trait is that in some prior meeting Gav advised against exhaustively modifying some core pallets like pallet-assets and instead recommended extending them. That is, precisely the reasoning for having created pallet-assets-freezer.

muharem · 2024-07-25T12:56:43Z

@pandres95 yes, it should be done as an extension. there is also a died hook, the new trait make sense to me now.
so I would only advise to have a single PR, since HeldBalance along does not bring anything, and only breaking changes.
if introduced and merged separately, it is possible to get the HeldBalance and the holder pallet in different release versions. Two separate audits would be required. also I think we can review it better in a single PR.

…allet-assets`

pandres95 · 2024-08-01T02:32:10Z

@muharem the pallet implementation is done and ready to review

pandres95

Change this (thanks @ggwpez)

substrate/frame/assets-holder/src/lib.rs

…wpez

…wpez for the suggestion)

…rency::unreserve` that are followed by a potential failure of `dead_account`. This would prevent cases where `refund` is called externally by another pallet, and not as a part of a direct extrinsic call within the pallet.

…illustrate the changes.

…ust to be extra careful.

pandres95 · 2024-11-12T12:25:09Z

@gui1117 AFAIK, I've already addressed the changes you requested on your review. Let me know if there's something additional you need from my end. 😁

substrate/frame/assets/src/types.rs

substrate/frame/assets/src/functions.rs

… any deposit / remove `with_storage_layer`.

# Conflicts: # Cargo.lock

…uharem

…uharem

…ges from `master`.

substrate/frame/assets/src/functions.rs

… `transfer_and_die`.

umbrella/Cargo.toml

gui1117 · 2024-12-09T06:10:41Z

substrate/frame/assets/src/functions.rs

@@ -575,7 +607,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {

 		// Execute hook outside of `mutate`.
 		if let Some(Remove) = target_died {
-			T::Freezer::died(id, target);
+			T::Freezer::died(id.clone(), target);
+			T::Holder::died(id, target);


This method is called from burn. I don't where it is check that the account is allowed to die. Are we ok that admin can kill the account even when there some on hold?

Not really.

This is the exact reason why we had such a nightmare working on the ensures before an account could die. Because it's mandatory that an account doesn't have assets on hold before declaring it as dead.

This is a hook, not intended to burn up resources, but to make proper cleanup steps on external systems (if any needed) whenever an account is marked as dead (hence, informing the account already died).

That's also why its Freezer counterpart is also called that way.

Maybe I forgot to put an ensure here? Let me check. 👀

should be ensure no freezes and holds for decrease_balance as well

Done on a37b00b.

muharem · 2024-12-09T12:04:24Z

substrate/frame/assets/src/functions.rs

+			// Pre-check that an account can die if is below min balance
+			if source_account.balance < details.min_balance {
+				source_died =
+					Some(Self::dead_account(source, details, &source_account.reason, false));


it does mutate the state, should be called ones

In this specific case, we're safe.

Since the block for Asset::try_mutate hasn't finished yet, failing the ensure at this point would cause no mutations for details.

Also, no dest account mutations happened here because we're doing this before the Account::try_mutate block.

Finally, mutations for source_account only get saved at the end, much later than this step (see lines 728, 732).

fn dead_account modifies the storage frame_system::Account and potentially anything.
Those are not reverted AFAICT

Let me write a couple of additional tests to assert something, because from what I've seen in previous changes I've done in this sense is that if you run these ensures before knowing the account might die is that you get false positives (the call fails, but it was never dying).

muharem · 2024-12-09T12:05:52Z

substrate/frame/assets/src/functions.rs

@@ -575,7 +607,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {

 		// Execute hook outside of `mutate`.
 		if let Some(Remove) = target_died {
-			T::Freezer::died(id, target);
+			T::Freezer::died(id.clone(), target);
+			T::Holder::died(id, target);


should be ensure no freezes and holds for decrease_balance as well

change(pallet-assets): define HeldBalance

a0f5015

pandres95 requested a review from a team as a code owner May 21, 2024 05:43

pandres95 marked this pull request as draft May 21, 2024 05:43

pandres95 mentioned this pull request May 21, 2024

Define and implement Hold for Assets #4315

Open

3 tasks

pandres95 added 5 commits June 10, 2024 11:29

change(pallet-assets): implement Holder to calculate reducible bala…

6d2aa57

…nce based on whether there's held assets or not

change: add Holder type to missing pallet_assets Config implementations

029e887

change(pallet-assets): call died hook for Holder whenever necessary

bf376a5

Merge branch 'master' into pallet-assets-holder

fe951fa

fix(pallet-asset-conversion-ops): missing Holder in mock config

89ea43a

pandres95 force-pushed the pallet-assets-holder branch from c627c51 to 89ea43a Compare June 17, 2024 16:53

pandres95 added 4 commits July 10, 2024 14:34

Merge branch 'master' into pallet-assets-holder

9d326b8

change(pallet-assets): introduce tests to check for holds and freezes…

c76b654

… working together

Merge branch 'master' into pallet-assets-holder

c5690e1

pandres95 marked this pull request as ready for review July 11, 2024 09:45

pandres95 requested review from athei and a team as code owners July 11, 2024 09:45

muharem reviewed Jul 25, 2024

View reviewed changes

change(pallet-assets-freezer): adjust mock and tests to changes in `p…

a4bb10c

…allet-assets`

pandres95 changed the title ~~[Assets] Define HeldBalance~~ [Assets] Implement pallet-assets-holder Jul 29, 2024

feat(pallet-assets-holder): pallet implementation

32fa5c1

paritytech-review-bot bot requested a review from a team August 1, 2024 02:28

Merge branch 'master' into pallet-assets-holder

fe0a5e5

pandres95 requested a review from muharem August 1, 2024 02:33

pandres95 added 2 commits July 31, 2024 22:44

fix: fmt / cargo fmt

b93337e

chore: umbrella

4e0ced7

pandres95 commented Nov 6, 2024

View reviewed changes

substrate/frame/assets-holder/src/lib.rs Outdated Show resolved Hide resolved

pandres95 added 3 commits November 6, 2024 10:19

change(pallet-assets-holder): comment events as docstring (thanks @gg…

17ba78c

…wpez for the suggestion)

fix(prdoc): extend explanation of changes in balance model to better …

c10123a

…illustrate the changes.

pandres95 requested a review from ggwpez November 6, 2024 16:31

pandres95 added 4 commits November 7, 2024 00:19

Merge branch 'master' into pallet-assets-holder

99ee590

fix(pallet-assets): add with_storage_layer on transfer_and_die. J…

4a55d5c

…ust to be extra careful.

fix(pallet-assets): fmt

12e9a42

fix(pallet-assets-freezer): fmt

8615a4c

gui1117 approved these changes Nov 13, 2024

View reviewed changes

muharem reviewed Nov 19, 2024

View reviewed changes

substrate/frame/assets/src/types.rs Outdated Show resolved Hide resolved

substrate/frame/assets/src/functions.rs Outdated Show resolved Hide resolved

fix(pallet-assets): ensure that an account can die before unreserving…

4f50052

… any deposit / remove `with_storage_layer`.

pandres95 force-pushed the pallet-assets-holder branch from 7105fc3 to 4f50052 Compare November 27, 2024 14:24

Merge branch 'master' into pallet-assets-holder

3fc7ac2

# Conflicts: # Cargo.lock

pandres95 requested a review from muharem November 27, 2024 14:31

change(pallet-assets): apply change in documentation suggestion by @m…

17e4b16

…uharem

pandres95 force-pushed the pallet-assets-holder branch from 3d79a10 to 17e4b16 Compare November 27, 2024 15:46

fix: re-include pallet-assets-holder on Cargo.lock after merging chan…

d0005af

…ges from `master`.

gui1117 reviewed Dec 9, 2024

View reviewed changes

substrate/frame/assets/src/functions.rs Outdated Show resolved Hide resolved

fix: ensure_account_can_die should happen before mutating dest on…

d6af473

… `transfer_and_die`.

pandres95 force-pushed the pallet-assets-holder branch from 48fe5d3 to 19e59ed Compare December 9, 2024 04:06

Merge branch 'master' into pallet-assets-holder

f2d33e0

pandres95 force-pushed the pallet-assets-holder branch from 19e59ed to f2d33e0 Compare December 9, 2024 04:21

gui1117 reviewed Dec 9, 2024

View reviewed changes

muharem reviewed Dec 9, 2024

View reviewed changes

pandres95 added 4 commits December 9, 2024 11:13

fix(pallet-assets): add ensure_account_can_die on decrease_balance

a37b00b

fix(umbrella): missing dependency for pallet-assets-holder / fmt

63dc190

Merge branch 'master' into pallet-assets-holder

9757514

fix(pallet-assets): unused import

cf07ea7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Assets] Implement `pallet-assets-holder` #4530

[Assets] Implement `pallet-assets-holder` #4530

pandres95 commented May 21, 2024 •

edited

Loading

bkchr commented Jul 24, 2024

muharem left a comment

pandres95 commented Jul 25, 2024 •

edited

Loading

muharem commented Jul 25, 2024 •

edited

Loading

pandres95 commented Aug 1, 2024

pandres95 left a comment

pandres95 commented Nov 12, 2024 •

edited

Loading

gui1117 Dec 9, 2024

pandres95 Dec 9, 2024 •

edited

Loading

muharem Dec 9, 2024

pandres95 Dec 9, 2024

muharem Dec 9, 2024

pandres95 Dec 9, 2024 •

edited

Loading

gui1117 Dec 10, 2024

pandres95 Dec 10, 2024

muharem Dec 9, 2024

[Assets] Implement pallet-assets-holder #4530

Are you sure you want to change the base?

[Assets] Implement pallet-assets-holder #4530

Conversation

pandres95 commented May 21, 2024 • edited Loading

bkchr commented Jul 24, 2024

muharem left a comment

Choose a reason for hiding this comment

pandres95 commented Jul 25, 2024 • edited Loading

muharem commented Jul 25, 2024 • edited Loading

pandres95 commented Aug 1, 2024

pandres95 left a comment

Choose a reason for hiding this comment

pandres95 commented Nov 12, 2024 • edited Loading

gui1117 Dec 9, 2024

Choose a reason for hiding this comment

pandres95 Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

muharem Dec 9, 2024

Choose a reason for hiding this comment

pandres95 Dec 9, 2024

Choose a reason for hiding this comment

muharem Dec 9, 2024

Choose a reason for hiding this comment

pandres95 Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

gui1117 Dec 10, 2024

Choose a reason for hiding this comment

pandres95 Dec 10, 2024

Choose a reason for hiding this comment

muharem Dec 9, 2024

Choose a reason for hiding this comment

[Assets] Implement `pallet-assets-holder` #4530

[Assets] Implement `pallet-assets-holder` #4530

pandres95 commented May 21, 2024 •

edited

Loading

pandres95 commented Jul 25, 2024 •

edited

Loading

muharem commented Jul 25, 2024 •

edited

Loading

pandres95 commented Nov 12, 2024 •

edited

Loading

pandres95 Dec 9, 2024 •

edited

Loading

pandres95 Dec 9, 2024 •

edited

Loading